fix(cli): preserve committed worktrees on forced shutdown - #189
Conversation
The exit handler registered for new worktrees would unconditionally delete the worktree when process.exit() was called before the normal preservation block could null out worktreeCleanup. This happened on: - Double Ctrl+C (force-stop) - Graceful-shutdown timeout (5s) - Orchestrator crash via die() Fix: the exit handler now checks orchestrator state before cleaning up. If commitCount > 0 or hasPendingCommitFailure is true, the worktree is preserved even when the normal code path was bypassed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c6782dc6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
kunchenguid
left a comment
There was a problem hiding this comment.
Speaking as Kun's firstmate:
Reviewed the force-shutdown worktree preservation. On current main process.on("exit") still deletes the worktree even when commits exist, because timeout process.exit runs before the preservation block. Re-checking orchestrator state in the exit handler and printing the path is the right no-work-lost fix. Zero-commit cleanup stays. No default-behavior change.
|
Speaking as Kun's firstmate: Corrective no-work-lost force-shutdown preservation; approved. After #191 merged this is CONFLICTING vs main. GitHub update-branch cannot auto-merge. I am not dropping either fix. Conflict resolve is in flight on the existing branch (maintainer-can-modify), file-only, no workflow rewrite, no rebase-first for a captain flag. Not escalating waiting-on-author. |
Resolve the AGENTS.md conflict by keeping README as the owner of user-facing worktree preservation text and retaining this PR's note that the rule must hold on every exit path. Keep both the Linux sleep re-exec worktree ownership handoff and force-shutdown preservation. Co-authored-by: Kun Chen <kunchenguid@users.noreply.github.com>
kunchenguid
left a comment
There was a problem hiding this comment.
Speaking as Kun's firstmate: conflict with #191 is resolved on this branch. Keeping Linux sleep re-exec worktree handoff and force-shutdown preservation. Required CI is green.
|
Speaking as Kun's firstmate: this is merged. Thank you @jasonqlwilliams-alt — really appreciate you taking the time on this. |
Intent
Land community PR #168 to stop gnhf from deleting a --worktree checkout that still holds commits when force shutdown bypasses normal cleanup, specifically the double-Ctrl+C and shutdown-timeout half of issue #167. #168 is the selected fix over #170 and that choice must not be reopened. Keep the change narrowly limited to force-shutdown preservation: do not fix, refactor, or otherwise alter the separate Linux sleep-prevention re-exec parent-exit defect owned by sibling task gnhf-worktree-selfdelete-d2. Preserve Eric Cao's original commits and authorship, adding only follow-up commits needed for correctness or validation. A forced shutdown after commits must preserve the worktree, while a forced shutdown with zero commits and no pending commit failure must still remove it. Before timeout exit, when the worktree is preserved, print its absolute path on its own clear, copyable line. Add a committed automated regression test scoped to the existing preservation logic: assert that the commit-bearing cleanup path leaves the worktree intact and reports its absolute path. Do not build new double-SIGINT testing infrastructure. Unit tests alone are not sufficient evidence: retain the narrower automated regression and put the completed manual signal-path reproduction in the PR body with exact steps and observations. Manual proof: temporarily set FORCE_EXIT_TIMEOUT_MS to 0 and temporarily make the mock opencode agent complete iteration 1 and hang on iteration 2; in a fresh temporary git repository with a temporary HOME, sanitized git config, telemetry disabled, e2e fixtures on PATH, and prevent-sleep off, launch the built CLI with prompt hang after first commit and agent opencode in worktree mode; wait for the first commit and second message:start, then send two SIGINT signals 20 ms apart. Before #168 the result was exit 130, one timeout hit, 2 branch commits, 0 worktree directories, and 1 worktree registration. With #168 the result was exit 130, one timeout hit, 2 branch commits, 1 worktree directory, and 2 worktree registrations. The zero-commit control used prompt slow cleanup and produced exit 130, one timeout hit, 0 worktree directories, and 1 registration. The contributed regression test originally passed without the fix, so it was strengthened to hold the orchestrator open through timeout and proven to fail on unfixed behavior. Keep the diff tight and reviewable, do not add em dashes, and keep all repository CI commands green. If rebasing onto current main incorporates sibling d2 such that #168's state guard is redundant or contradictory, stop for a needs-decision keyed pr168-superseded-by-d2 rather than silently dropping or retaining it.
What Changed
--worktreecheckouts during exit cleanup when the run has commits or a pending commit failure, including forced-shutdown paths.Risk Assessment
✅ Low: The change narrowly preserves commit-bearing or commit-failure worktrees across forced exit paths, retains zero-commit cleanup, reports the absolute preserved path before timeout exit, and adds behavior-oriented regression coverage without altering the separate sleep-prevention path.
Testing
The focused regression and zero-commit signal control passed. The built-CLI double-SIGINT reproduction exited 130 with one timeout, two branch commits, one preserved worktree directory, two registrations, and its absolute path printed on a clear line; all temporary testing edits were restored.
Evidence: Double-SIGINT manual reproduction
Exit 130; one timeout; two branch commits; one preserved worktree directory; two worktree registrations; absolute path reported.Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
pnpm exec vitest run src/cli.test.ts -t "preserves and reports a worktree with commits when exit handler fires before preservation block"pnpm run build && pnpm exec vitest run e2e/e2e.test.ts -t "cleans up the worktree when no changes are made in --worktree mode"Temporarily setFORCE_EXIT_TIMEOUT_MSto 0 and made the mock OpenCode agent hang on iteration 2, then rannode /tmp/no-mistakes-evidence/01KZKC7FTMKEPYFQ73T0139REG/manual-signal-repro.mjsagainst the built CLI and sent two SIGINT signals 20 ms apartgit status --short && git diff -- src/cli.ts e2e/fixtures/mock-opencode-server.mjsafter restoring temporary instrumentation✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.